nACK

Thanks for this, but honestly I'm not convinced there's a need for it. Overloading this function to add a default nRounds = 0 doesn't really give us much and this just adds extra code without much (if any) benefit. I'd like to stick with cleaner and less code if possible, not more.

Changed back comment, after rereading the old one is fine. fixed alignment, darn tabbing. removed the .gitnore change, thanks for explaining why. That reasoning makes sense

I personally disagree with it not providing any value. When reading the code, it makes no sense for that zero to be there, and requires the reader to look into the function and figure out what that zero actually means, whereas it is clear why the output is there and what it does. Also, in regards to writing, it becomes quite possible to forget that the zero needs to be there, again wasting time. I personally messed that up in a prior PR because it doesn't intuitively make sense for the zero to be present.

requires the reader to look into the function and figure out what that zero actually means

That should be the case regardless for any contributors to the codebase.

it becomes quite possible to forget that the zero needs to be there, again wasting time

That's why function signatures exist and are checked at compile time.

Alright, well. I've made my case, I think this makes it easier to read without adding any real negatives. You guys can decide how you wish :)

I personally disagree with it not providing any value. When reading the code, it makes no sense for that zero to be there, and requires the reader to look into the function and figure out what that zero actually means, whereas it is clear why the output is there and what it does. Also, in regards to writing, it becomes quite possible to forget that the zero needs to be there, again wasting time. I personally messed that up in a prior PR because it doesn't intuitively make sense for the zero to be present.

This is not a valid argument. Any C++ developer will understand the functions they're calling before using them. For complicated code, refactoring makes sense, but this is just setting a default.

Comments can serve to help understand why a block of code is written the way it is, or what the intended use of a variable is, but "forgetting" a parameter alone is a terrible excuse for a function overload. Most IDEs will detect this and certainly the compiler does at compile time.

There's really not any excuse for not understanding the code you're contributing to.

This is not a valid argument. Any C++ developer will understand the functions they're calling before using them. For complicated code, refactoring makes sense, but this is just setting a default.

Sure, and this is nothing major. But I don't understand how you don't see the slight improvement this makes to readability. Seeing the output there makes complete sense, seeing a zero there doesn't and forces you to spend time figuring out what that zero actually does. Same with usage, I expect to pass an output. I don't expect to pass some int, wasting time forcing me to see what exactly that int does. Sure a good dev will know what the method does and how exactly to manipulate it, but people forget and some devs aren't as good as you all 😆.

The reason I think this is a valid change is that the second parameter (i think) will never be used externally. It will always be zero, only in the recursive loop will it change. It seems a lot cleaner to me to overload it.

Most IDEs will detect this and certainly the compiler does at compile time.

Sure IDEs will catch it or the compiler will. But again, that wastes time. I just don't see the point for it.

There's really not any excuse for not understanding the code you're contributing to.

Of course you should understand it as much as you can. But I think it is also contributors' job to make sure that it is easy to understand for people who aren't as familiar, which I think this change does without any negative effects

@paulied I see what you are trying to achieve but imo it's a weird way (no offence :) ).

Given that I'd rather go with smth like

     int  CountInputsWithAmount(CAmount nInputAmount);
 
     // get the PrivateSend chain depth for a given input
-    int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRounds) const;
+    int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRounds = 0) const;
     // respect current settings
    int GetOutpointPrivateSendRounds(const COutPoint& outpoint) const;
 

In fact, if you really want to fix readability of Get*OutpointPrivateSendRounds() usage, I would go even further with smth like

     int  CountInputsWithAmount(CAmount nInputAmount);
 
     // get the PrivateSend chain depth for a given input
-    int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRounds) const;
+    int GetRealOutpointPrivateSendRounds(const COutPoint& outpoint, int nRounds = 0) const;
     // respect current settings
-    int GetOutpointPrivateSendRounds(const COutPoint& outpoint) const;
+    int GetCappedOutpointPrivateSendRounds(const COutPoint& outpoint) const;
 
     bool IsDenominated(const COutPoint& outpoint) const;
 

and then would refactor all the code using these functions, so that it was 100% clear what number we are using exactly in each particular case. Or smth like that, more or less :)

Refactored based on @UdjinM6's response. I forgot about setting a default parameter, never used it much before. Thanks for that. Definitely better then my approach
I think the suggestion to make the other one 'Capped' is quite good and describes what the method actually does better

forces you to spend time figuring out what that zero actually does
wasting time forcing me to see what exactly that int does

I don't agree that it's a waste of time. It makes sense to have a holistic, not partial, understanding of the codebase. There might be a good reason to use that one random parameter which you're sure in this case will never be used "externally". (I'm honestly not sure what you mean by "externally" here, unless you mean referenced outside the class itself.)

Udjin's suggestion of using a default param is of course cleaner (but I would have opted to just add that to the existing method rather than add a new method). But I still don't see any readability issues with the original, and I think it's a reasonable expectation that contributors either be experienced with C++ development or quickly learn those skills.

@UdjinM6, I think that is good now, sorry for the mess of commits.

Mostly looks good at 635de9b -- I just see some trailing spaces in one line that should be removed.

@paulied no worries, that's what PR branches are for :)
BTW: I'd suggest to look into squashing commits with git, that way you can keep your PRs "clean"

And last but not least: Thanks for contributing, much appreciated.

Extra spacing removed.

Thanks @schinzelh, thanks for the tip. Glad to be able to help out, hopefully xd

Thanks @UdjinM6 :)